-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds getPaymentMethods, saveInvoice, getTags, getSevUsers, getContactAddresses, getStaticCountries, getParts #2
Conversation
Awesome! Going to review it this week. Sorry for the delay :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Thank you for contributing new methods :)
The code itself looks good. You stick to the existing conventions 👍
Also thank you for adding tests ❤️
Unfortunately, we can't execute mutations in our CI system because we don't have a dedicated test system for SevDesk :(. We have the same problem with the addDocument()
method.
Until we have a dedicated test system, these tests need to be skipped and can only be executed in manual tests. I tried to change your PR but it seems like I can't push changes to your main
branch. Better create dedicated PR branches in your fork next time 😁
That's why I created a follow-up PR with your commits and my changes: #3
@@ -63,7 +70,7 @@ export class SevDeskClient { | |||
throw error; | |||
} | |||
if (response.ok === false || error) { | |||
const message = error?.message ?? body?.error?.message; | |||
const message = error?.message ?? body?.error?.message ?? body.message; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seriously? 😁 They also return just {"message": "..."}
?
No description provided.